Skip to content

refactor(Example): TabsContainer and StackContainer refactor#3925

Merged
sgaczol merged 6 commits intomainfrom
@sgaczol/containers-refactor
Apr 30, 2026
Merged

refactor(Example): TabsContainer and StackContainer refactor#3925
sgaczol merged 6 commits intomainfrom
@sgaczol/containers-refactor

Conversation

@sgaczol
Copy link
Copy Markdown
Collaborator

@sgaczol sgaczol commented Apr 22, 2026

Description

Previously, we were storing the actual React Component references directly inside the useReducer state (within TabRoute and StackRoute objects). This PR separates the navigation state from the Components. During the render phase, the containers dynamically resolve the correct Component by matching the route name from the state against a componentsByName Map (built via useMemo from the routeConfigs prop), rather than storing Component references in reducer state.

There is one issue related to StackContainer: often in our environment, we wrap StackContainer in an intermediate component (e.g. StackSetup) to consume contexts such as useToast(). If that wrapper is not explicitly exported from its file, Fast Refresh causes useReducer inside StackContainer to re-fire its initialization function, resetting the stack to the first screen. This issue will be solved in a separate PR.

Changes

  • Update TabRoute and StackRoute types to omit Component from the route state
  • Update TabsContainer and StackContainer to resolve components via a componentsByName Map during render

Before & after - visual documentation

N/A

Test plan

N/A

Checklist

  • Included code example that can be used to test this change.
  • For visual changes, included screenshots / GIFs / recordings documenting the change.
  • For API changes, updated relevant public types.
  • Ensured that CI passes

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the Gamma TabsContainer and StackContainer to keep reducer/navigation state free of React component references, and instead resolve the latest Component from routeConfigs during render to improve Fast Refresh behavior.

Changes:

  • Update TabRoute / StackRoute runtime types to omit Component from reducer state.
  • Strip Component when creating routes in the tabs/stack reducers.
  • Resolve the correct Component per rendered route by matching route.name against routeConfigs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
apps/src/shared/gamma/containers/tabs/reducer.tsx Omits Component from tab route state when initializing routes.
apps/src/shared/gamma/containers/tabs/TabsContainerItem.types.ts Adds Component prop to TabsContainerItem props.
apps/src/shared/gamma/containers/tabs/TabsContainerItem.tsx Renders using the resolved Component prop instead of reading from route state.
apps/src/shared/gamma/containers/tabs/TabsContainer.types.tsx Updates TabRoute type to omit Component.
apps/src/shared/gamma/containers/tabs/TabsContainer.tsx Resolves Component from routeConfigs by route name at render time.
apps/src/shared/gamma/containers/stack/reducer.tsx Omits Component from stack route state when creating routes.
apps/src/shared/gamma/containers/stack/StackContainer.types.tsx Updates StackRoute type to omit Component (and adjusts field delimiter).
apps/src/shared/gamma/containers/stack/StackContainer.tsx Resolves Component from routeConfigs by route name at render time.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 7 to 9
pendingForUpdate: boolean;
Component: React.ComponentType;
}
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React.ComponentType is referenced here but this file doesn't import React, which will fail typechecking in this codebase (other files importing React when using the namespace). Import React as a type, or switch the prop type to an imported ComponentType/TabRouteConfig['Component'].

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, import React from 'react';

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: 047e2d1

activityMode: StackScreenProps['activityMode'];
routeKey: StackScreenProps['screenKey'];
isMarkedForDismissal: Boolean, // whether this route is during or after dismissal process
isMarkedForDismissal: Boolean; // whether this route is during or after dismissal process
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the boxed Boolean type. Prefer the primitive boolean here to avoid allowing new Boolean(false)-style truthy values and to match typical TS usage.

Suggested change
isMarkedForDismissal: Boolean; // whether this route is during or after dismissal process
isMarkedForDismissal: boolean; // whether this route is during or after dismissal process

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hes right, treat is as a typo and fix it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done: 047e2d1

Comment on lines +82 to +92
const matchingConfig = routeConfigs.find(
config => config.name === route.name,
);
if (!matchingConfig) {
throw new Error(
`[Tabs] No config matches the "${route.name}" route name`,
);
}

const Component = matchingConfig.Component;

Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routeConfigs.find(...) is executed once per route on every render, making this O(n²) in the number of tabs. Consider precomputing a Map/object lookup (e.g., configByName) with useMemo and doing O(1) lookups inside the .map.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +83
const matchingConfig = routeConfigs.find(
config => config.name === name,
);
if (!matchingConfig) {
throw new Error(`[Stack] No config matches the "${name}" route name`);
}

const Component = matchingConfig.Component;

Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routeConfigs.find(...) runs for every rendered stack screen on each render, which becomes O(n²) as the stack grows. Consider memoizing a name->config map and doing constant-time lookups here.

Copilot uses AI. Check for mistakes.
Comment on lines +138 to +139
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { Component, ...rest } = config;
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using eslint-disable to drop Component from the returned route. A small refactor can keep linting enabled (e.g., rename the destructured field to an underscore-prefixed identifier if that's allowed by the lint config, or explicitly consume it with void), while still omitting it from rest.

Suggested change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { Component, ...rest } = config;
const { Component, ...rest } = config;
void Component;

Copilot uses AI. Check for mistakes.
Comment on lines +280 to +281
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { Component, ...rest } = config;
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the eslint-disable here; it’s better to omit Component without disabling lint (e.g., rename the destructured field to an ignored identifier per lint rules, or explicitly consume it) while still returning ...rest.

Suggested change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { Component, ...rest } = config;
const { Component: _Component, ...rest } = config;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave it in the current form, because it's on the example app's side and we also did the same in other places.

@sgaczol sgaczol force-pushed the @sgaczol/containers-refactor branch from 881b3f0 to ced4f0c Compare April 22, 2026 12:03
@t0maboro
Copy link
Copy Markdown
Contributor

t0maboro commented Apr 22, 2026

For stack v5 tests, I'm observing that fast refresh is resetting the stack to the initial route. I'm on this commit: ced4f0c

stack-v5.mov

Comment on lines +280 to +281
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { Component, ...rest } = config;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave it in the current form, because it's on the example app's side and we also did the same in other places.

Comment thread apps/src/shared/gamma/containers/stack/StackContainer.tsx Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 268 out of 269 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through each test one by one, but I've left few general remarks below. Please answer them.

Comment on lines +28 to +34
const componentsByName = React.useMemo(() => {
const map = new Map<string, StackRouteConfig['Component']>();
for (const config of routeConfigs) {
map.set(config.name, config.Component);
}
return map;
}, [routeConfigs]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to separate helper hook. Similarly to useSanitizeRouteConfigs - you can define it in the bottom of this file.

If this is reused between different containers (Stack & Tabs) -> then you can move ti to standalone file and share it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for generic helper

activityMode: StackScreenProps['activityMode'];
routeKey: StackScreenProps['screenKey'];
isMarkedForDismissal: Boolean, // whether this route is during or after dismissal process
isMarkedForDismissal: Boolean; // whether this route is during or after dismissal process
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hes right, treat is as a typo and fix it.

Comment on lines +32 to +38
const componentsByName = React.useMemo(() => {
const map = new Map<string, TabRouteConfig['Component']>();
for (const config of routeConfigs) {
map.set(config.name, config.Component);
}
return map;
}, [routeConfigs]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it is shared. Move it to standalone file and share it between implementations if possible. If you'd have to do some weird things to satisfy type constraints -> then make a hook per container, but definitively move it to a hook.

Comment on lines 7 to 9
pendingForUpdate: boolean;
Component: React.ComponentType;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, import React from 'react';

}

const STACK_ROUTE_CONFIGS: StackRouteConfig[] = [
export const STACK_ROUTE_CONFIGS: StackRouteConfig[] = [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to export that, or only components?

I'm aware that we said to literally export everything during the meeting, but realising now that I had hidden assumption and thought only of components.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think that exporting components is all we need there, so I'll change it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to apply these exports rules also to places where we had no problems? What do you think?

cc @t0maboro?

Copy link
Copy Markdown
Contributor

@t0maboro t0maboro Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if examples touching stack v4 are working fine, we should consider applying rules only to specific directories

Copy link
Copy Markdown
Collaborator Author

@sgaczol sgaczol Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, we need to refactor issue-tests then, so tests regarding different components are in different directories to implement such approach. However I think that such change should be added in a separate PR and if so, an addition of a new ESLint rule should be moved to that PR. What do you think?

Comment thread eslint-plugin-local-rules/index.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to not make top-level directory out of it. Is it possible to put it inside apps?

Copy link
Copy Markdown
Contributor

@t0maboro t0maboro Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicitly disabling in top-level .eslintrc and overriding that rule in the nested directory .eslintrc should work, we may consider adding it deeper, even for SFTs and CITs only, because issue tests won't be actively updated once written. wdyt?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that issue tests regarding Tabs or Stackv5 created in the future may seem to be broken, if we don't apply that rule to at least some of the issue tests. We could divide files in issue-tests into a few directories and apply the rule only to some of them.

sgaczol added 3 commits April 27, 2026 13:41
now, instead of keeping a Component in state, we find one in routeConfigs prop, basing on routeKey
also small change in TabsContainer
@sgaczol sgaczol force-pushed the @sgaczol/containers-refactor branch from b73fe14 to 48b37b7 Compare April 27, 2026 11:55
@sgaczol sgaczol requested a review from Copilot April 27, 2026 12:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@sgaczol sgaczol force-pushed the @sgaczol/containers-refactor branch 2 times, most recently from 4990b5c to 3ba527a Compare April 27, 2026 18:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/src/shared/gamma/containers/tabs/TabsContainer.tsx Outdated
Copy link
Copy Markdown
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked the runtime but it looks good.

Remember to update the PR description.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@sgaczol sgaczol merged commit d3abbe7 into main Apr 30, 2026
3 checks passed
@sgaczol sgaczol deleted the @sgaczol/containers-refactor branch April 30, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants